base: Add media retention policy to EventCacheStore #3918
Conversation
zecakeh
force-pushed
the
event-cache-retention-policy
branch
from
1ff7a2a
to
b3b61fe
Compare
|
Sounds awesome but it's a biggie so I think it's safer to pass it over to somebody more familiar with the SDK, congrats Benji! 😁 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3918 +/- ##
==========================================
+ Coverage 84.18% 84.33% +0.15%
==========================================
Files 267 268 +1
Lines 28036 28554 +518
==========================================
+ Hits 23601 24082 +481
- Misses 4435 4472 +37 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kévin Commaille <[email protected]>
zecakeh
force-pushed
the
event-cache-retention-policy
branch
from
84d83d1
to
02a3630
Compare
|
Rebased on main to solve a conflict. |
|
Hi, thanks for the PR, since this is a large commit it might take a bit of time to get to review it. I'll try my best. |
There was a problem hiding this comment.
My main feedback is that I wouldn't want to have external users have to deal with a EventCacheStoreWrapper, but rather with the EventCacheStore; so EventCacheStore (the trait) would likely need to be renamed into something else, maybe EventCacheStoreBackend or EventCacheStoreImpl, or something else that makes this clear enough?
I haven't looked at the code in details, so forwarding review to somebody else.
|
I know this is a big commit, so for reviewers I am going to reiterate what I said in the Matrix room to help reviewing part by part: I would recommend starting in Then in any order:
|
|
Before reviewing this PR, I'm wondering: don't we have an MSC about media retention policy? I see there is MSC1763. I don't know the state of this MSC, but I think we should link both (a media is an event after all). I don't know if the What do you think about this? |
|
This is on purpose a device-specific setting. The sole goal here is for the client or the user (if the client offers a setting for it) to be allowed to limit the size of the cache on the device. This kind of setting can usually change from one device to the other, e.g. the limitations would be more strict on an phone where disk space is limited than on a computer with a lot of free space. |
There was a problem hiding this comment.
I like the general idea and the MediaPolicy type. I'm suggesting to rewrite the history to make it clearer. I'm also suggesting to introduce a new MediaService API instead of adding a new event cache indirection with EventCacheStoreWrapper. What do you think?
Sorry for the late review.
| @@ -93,7 +92,7 @@ pub struct BaseClient { | |||
| /// Database | |||
| pub(crate) store: Store, | |||
| /// The store used by the event cache. | |||
| event_cache_store: Arc<DynEventCacheStore>, | |||
| event_cache_store: EventCacheStoreWrapper, | |||
There was a problem hiding this comment.
This is now an EventCacheStoreLock:
matrix-rust-sdk/crates/matrix-sdk-base/src/event_cache/store/mod.rs
Lines 42 to 52 in 4724648
which is a cross-process lock.
| /// The retention policy for media content used by the `EventCacheStore`. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct MediaRetentionPolicy { |
There was a problem hiding this comment.
Can we extract the creation of this type inside its own commit please?
There was a problem hiding this comment.
Also, this type and its implementation is quite long. It must be inside its own module.
| pub max_cache_size: Option<usize>, | ||
| /// The maximum authorized size of a single media content, in bytes. |
There was a problem hiding this comment.
| pub max_cache_size: Option<usize>, | |
| /// The maximum authorized size of a single media content, in bytes. | |
| pub max_cache_size: Option<usize>, | |
| /// The maximum authorized size of a single media content, in bytes. |
| pub max_file_size: Option<usize>, | ||
| /// The duration after which unaccessed media content is considered |
There was a problem hiding this comment.
| pub max_file_size: Option<usize>, | |
| /// The duration after which unaccessed media content is considered | |
| pub max_file_size: Option<usize>, | |
| /// The duration after which unaccessed media content is considered |
| /// If this is set, media content whose last access is older than this | ||
| /// duration will be removed from the media cache during a cleanup. | ||
| /// | ||
| /// Defaults to 30 days. |
There was a problem hiding this comment.
I think 60 days is theoretically a better default for this value.
|
|
||
| /// The retention policy for media content used by the `EventCacheStore`. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct MediaRetentionPolicy { |
There was a problem hiding this comment.
Can you add unit tests for this type please?
| @@ -29,43 +29,81 @@ pub trait EventCacheStore: AsyncTraitDeps { | |||
| /// The error type used by this event cache store. | |||
| type Error: fmt::Debug + Into<EventCacheStoreError>; | |||
|
|
|||
| /// Add a media file's content in the media store. | |||
| /// The retention policy set to cleanup the media cache. | |||
| async fn media_retention_policy(&self) -> Result<Option<MediaRetentionPolicy>, Self::Error>; | |||
There was a problem hiding this comment.
This addition can go in a second commit:
- First commit: add
MediaRetentionPolicy - Second commit: use
MediaRetentionPolicyalong with tests.
| /// | ||
| /// [`EventCacheStore`]: super::EventCacheStore | ||
| #[derive(Debug, Clone)] | ||
| pub struct EventCacheStoreWrapper { |
There was a problem hiding this comment.
I understand why you've chosen this design.
I'm not comfortable with this though for several reasons, the major one being it introduces another type indirection for the EventCacheStore itself. To reach the EventCacheStore, the entry point is EventCacheStoreWrapper, then DynEventCacheStore, then EventCacheStore. If we include the EventCacheStoreLock, it introduces another level of indirection.
Note that this is only for media management for the moment.
Possible solutions:
-
Use standalone functions to be used by the multiple
EventCacheStoreimplementations, liek we did for the cross-process lock andMemoryStore: 94bd421 -
Use a standalone type to manage media, like
MediaService, which takes aMediaRetentionPolicy. TheEventCacheStorewould delegate the handling of media toMediaService, such as:pub async fn add_media_content(&self, request: &MediaRequest, content: Vec<u8>) -> Result<()> { self.media_service.add_media_content(&self, request).await }
That way we can test MediaService alone, it's a self-contained API, and it removes the need for another EventCacheWrapper type + type indirection.
Bonus: MediaService can be introduced inside its own commit, along with its tests. Another commit can use MediaService inside the EventCacheStore implementations.
What do you think?
| @@ -187,27 +188,27 @@ impl SqliteAsyncConnExt for SqliteAsyncConn { | |||
| } | |||
|
|
|||
| pub(crate) trait SqliteTransactionExt { | |||
| fn chunk_large_query_over<Query, Res>( | |||
| fn chunk_large_query_over<K, Query, Res>( | |||
There was a problem hiding this comment.
| fn chunk_large_query_over<K, Query, Res>( | |
| fn chunk_large_query_over<Key, Query, Res>( |
| } | ||
|
|
||
| impl<'a> SqliteTransactionExt for Transaction<'a> { | ||
| fn chunk_large_query_over<Query, Res>( | ||
| fn chunk_large_query_over<K, Query, Res>( |
There was a problem hiding this comment.
| fn chunk_large_query_over<K, Query, Res>( | |
| fn chunk_large_query_over<Key, Query, Res>( |
Allows to prevent the media cache from growing indefinitely by setting different limitations like size or last access time. I tried to select sensible defaults for those values.
Currently cleanups must be triggered manually, a future PR will add the possibility to run a regular cleanup task so they happen automatically.